Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some assorted region hashing fixes. #43743

Merged
merged 2 commits into from
Aug 11, 2017
Merged

Some assorted region hashing fixes. #43743

merged 2 commits into from
Aug 11, 2017

Conversation

michaelwoerister
Copy link
Member

This PR contains three changes.

  1. It changes what we implement HashStable for. Previously, the trait was implemented for things in the local TyCtxt. That was OK, since we only invoked hashing with a TyCtxt<'_, 'tcx, 'tcx> where there is no difference. With query result hashing this becomes a problem though. So we now implement HashStable for things in 'gcx.
  2. The PR makes the regular HashStable implementation not anonymize late-bound regions anymore. It's a waste of computing resources and it's not clear that it would always be correct to do so.
  3. The PR adds an option for stable hashing to treat all regions as erased and uses this new option when computing the TypeId. This should help with Tracking issue for non_static_type_id #41875.

I did not add a test case for (3) since that's not possible yet. But it looks like @zackmdavis has something in the pipeline there :).

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Aug 8, 2017 •
edited
Loading

not anonymize late-bound regions anymore. It's a waste of computing resources and it's not clear that it would always be correct to do so.

This will break for<'a> fn(&'a T) -> &'a U sharing a TypeId with for<'b> fn(&'b T) -> &'b U which is a feature. But it's only a TypeId feature, I'm worried about conflating it with HashStable.

@michaelwoerister
Copy link
Member Author

This will break for<'a> fn(&'a T) -> &'a U sharing a TypeId with for<'b> fn(&'b T) -> &'b U

No it shouldn't because all regions are erased when computing the TypeId.

@michaelwoerister
Copy link
Member Author

But maybe I read the corresponding RFC wrong and TypeId should act as if everything was anonymized instead of erased?

@eddyb
Copy link
Member

eddyb commented Aug 8, 2017

@michaelwoerister Are you saying for<'a> fn(&'a T) -> &'a U and fn(&'static T) -> &'static U have the same TypeId? That means they can be transmuted by Any...

@michaelwoerister
Copy link
Member Author

I see that that's a problem -- yet the assert_eq!(TypeId::of::<ContainsRef<'a>>(), TypeId::of::<ContainsRef<'static>>()) from the RFC thread suggests that they should be treated the same. I kind of don't get why we want to allow non-static lifetimes in type_id() anyway.

@eddyb
Copy link
Member

eddyb commented Aug 8, 2017

@michaelwoerister That's completely different from for<'a> lifetimes (which shouldn't get erased).

@michaelwoerister
Copy link
Member Author

That's completely different from for<'a> lifetimes

But if we did not have the 'static bound on Any, it could lead to the same kind of problems, right? That is, put in a ContainsRef<'a> and take out a ContainsRef<'static>. Or am I missing something here?

@eddyb
Copy link
Member

eddyb commented Aug 8, 2017

@michaelwoerister Correct. for<'a> fn(&'a T) -> &'a U is 'static and rightly so.

@michaelwoerister
Copy link
Member Author

I changed the last commit to just call tcx.erase_regions() before computing the type-id hash. That should have the desired effects: erase free regions, anonymize bound regions.

@eddyb
Copy link
Member

eddyb commented Aug 9, 2017

@michaelwoerister I still see a removed anonymize_late_bound_regions call.

@michaelwoerister
Copy link
Member Author

@eddyb Yes, the one in impls_ty.rs is removed intentionally. In the general case it might not be valid to anonymize. Once we start caching these things, I rather want these hashes to be accurate and there's no real need to optimize for a case where someone changes region names.

@eddyb
Copy link
Member

eddyb commented Aug 9, 2017

@michaelwoerister Okay can you make sure we still produce the same TypeId (and add a test if one is missing) for fn(for<'a> fn(&'a T) -> &'a U) and fn(for<'b> fn(&'b T) -> &'b U)?
IIRC anonymize_late_bound_regions doesn't handle nesting.

@michaelwoerister
Copy link
Member Author

Done!

@eddyb
Copy link
Member

eddyb commented Aug 9, 2017

r=me I guess (if the test passes)

@michaelwoerister
Copy link
Member Author

@bors r=eddyb

Thanks, @eddyb!

@bors
Copy link
Contributor

bors commented Aug 9, 2017

📌 Commit 6dbd846 has been approved by eddyb

@aidanhs aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Aug 10, 2017
@bors
Copy link
Contributor

bors commented Aug 11, 2017

⌛ Testing commit 6dbd846 with merge 73c3f55...

bors added a commit that referenced this pull request Aug 11, 2017
@bors
Auto merge of #43743 - michaelwoerister:gcx-tcx-switcheroo, r=eddyb
73c3f55 
Some assorted region hashing fixes.

This PR contains three changes.
1. It changes what we implement `HashStable` for. Previously, the trait was implemented for things in the local `TyCtxt`. That was OK, since we only invoked hashing with a `TyCtxt<'_,  'tcx, 'tcx>` where there is no difference. With query result hashing this becomes a problem though. So we now implement `HashStable` for things in `'gcx`.
2. The PR makes the regular `HashStable` implementation *not* anonymize late-bound regions anymore. It's a waste of computing resources and it's not clear that it would always be correct to do so.
3. The PR adds an option for stable hashing to treat all regions as erased and uses this new option when computing the `TypeId`. This should help with #41875.

I did not add a test case for (3) since that's not possible yet. But it looks like @zackmdavis has something in the pipeline there `:)`.

r? @eddyb
@bors
Copy link
Contributor

bors commented Aug 11, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 73c3f55 to master...

@bors bors merged commit 6dbd846 into rust-lang:master Aug 11, 2017
@bors bors mentioned this pull request Aug 11, 2017
Merged
@zackmdavis zackmdavis mentioned this pull request Aug 25, 2017
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@michaelwoerister @eddyb @bors @aidanhs